Migrate away from c-t-go/x509 and c-t-go/asn1 #198

phbnf · 2025-03-18T10:55:16Z

Towards #44

This PR migrates away from certificate-transparency-go/x509 and certificate-transparency-go/asn1. A follow up PR will drop dependency on certificate-transparency-go/tls.

To allow for such a transition, this PR does a few things:

modifies how certificates and their fields are printed: c-t-go/x509 has a lot of methods to print certificates and their fields. crypto/x509 does not have methods to print full certs anymore, but individual fields that implement the stringer interface can be printed. We only every used full certificate printing in chain_validation_test.go, so I've simply replaced this with printing its md5sum for now. This allows to delete the full certificate printing methods. If we need them again, we can use other libraries.
moves BuildPrecertTBS from c-t-go/x509 to x509util/ct
changes how isPreIssuer works, since the crypto/x509 library won't parse CT extended key usage extensions.

# Conflicts: # internal/scti/storage.go # Conflicts: # internal/scti/ctlog.go # Conflicts: # internal/scti/chain_validation.go # internal/scti/handlers_test.go # internal/scti/signatures_test.go # Conflicts: # ctlog.go

# Conflicts: # internal/scti/requestlog.go

# Conflicts: # ctlog.go # Conflicts: # ctlog.go # Conflicts: # internal/scti/chain_validation_test.go

# Conflicts: # internal/x509util/x509util.go # Conflicts: # internal/x509util/x509util.go

# Conflicts: # internal/scti/handlers.go

# Conflicts: # internal/x509util/ct.go # internal/x509util/ct_test.go

AlCutter

Tangential thought; is types really two separate packages, e.g. rfc6962 and staticct?

AlCutter · 2025-03-18T11:30:22Z

internal/scti/chain_validation.go

+	//  - allow pre-certificates and chains with pre-issuers
+	//  - allow certificate without policing them since this is not CT's responsibility
+	// See /internal/x509fork/README.md for further information.
+	verifyOpts := x509fork.VerifyOptions{


Naming idea for this fork package: lax509 :)

Sending a different PR to avoid making this one even larger.

AlCutter · 2025-03-18T11:36:32Z

internal/scti/chain_validation_test.go

@@ -690,7 +692,7 @@ func TestPreIssuedCert(t *testing.T) {
 				t.Fatalf("failed to ValidateChain: %v", err)
 			}
 			for i, c := range chain {
-				t.Logf("chain[%d] = \n%s", i, x509util.CertificateToString(c))
+				t.Logf("chain[%d] = \n%s", i, md5.Sum(c.Raw))


md5?

Would printing the Subject make it easier to tie back to the certs in the chain?

AlCutter · 2025-03-18T11:37:28Z

internal/scti/handlers.go

+func isPreIssuer(cert *x509.Certificate) bool {
+	// Look for the extension in the Extensions field and not ExtKeyUsage
+	// since crypto/x509 does not recognize this extension as an ExtKeyUsage.
+	for _, ext := range cert.Extensions {
+		if types.OIDExtKeyUsageCertificateTransparency.Equal(ext.Id) {
+			return true
+		}
+	}
+	return false


I've seen this in a couple of places, feels like it might reasonably live exported in x509util?

Yeah - there's a TODO to merge them, I'll do this in a followup PR. This PR was 90% done before the other isPreissuer was added.

AlCutter · 2025-03-18T11:39:54Z

internal/x509util/ct.go

+var (
+	oidExtensionAuthorityKeyId = asn1.ObjectIdentifier{2, 5, 29, 35}
+	// These extensions are defined in RFC 6962 s3.1.
+	oidExtensionCTPoison                        = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 11129, 2, 4, 3}
+	oidExtensionKeyUsageCertificateTransparency = asn1.ObjectIdentifier{1, 3, 6, 1, 4, 1, 11129, 2, 4, 4}
+)


could/should these live next to the other RFC6962 OIDs currently in types?

They could, yes: but what's the benefit?

I decided to leave them here because:

we're only talking about two vars, defined in the RFC that and haven't changed in 10+ years

it makes this file more readable

x509util is really what it says: it's a util for x509 and it does not import anything else, it just depends on standard libraries.

AlCutter · 2025-03-18T11:41:38Z

internal/x509util/ct.go

+	for i, ext := range tbs.Extensions {
+		if ext.Id.Equal(oid) {
+			if extAt != -1 {
+				return nil, errors.New("multiple extensions of specified type present")
+			}
+			extAt = i
+		}
+	}


I think you may be able to use slices.IndexFunc here?

I can yeah. I'll leave it for now in this PR since these methods are being copied from c-t-go, and can send a followup PR.

I find that the end result is not necessarily easier to read though, especially here since we need to use the function twice to find duplicate extensions. The current implementation only requires one loop iteration, this one two. Plus, the base implementation of slices.Index slices.IndexFunc aren't really bringing any magic. I've done a fair amount of function programming before, and I really really really like and the slice package, but for very simple usecases like this I find it way less readable.

Ack - will do in a followup PR, this PR is trying no to change the implementation of these functions too much and to copy them over from c-t-go.

AlCutter · 2025-03-18T11:42:42Z

internal/x509util/ct.go

+		for i, ext := range tbs.Extensions {
+			if ext.Id.Equal(oidExtensionAuthorityKeyId) {
+				keyAt = i
+				break
+			}
+		}


might be able to use slices.IndexFunc here?

Ack - will do in a followup PR, this PR is trying no to change the implementation of these functions too much and to copy them over from c-t-go.

roger2hk · 2025-03-18T11:35:17Z

internal/x509fork/cert_pool.go

-//
-// Deprecated: if s was returned by [SystemCertPool], Subjects
-// will not include the system roots.
+// Undeprecated in this fork.


It would be weird to have only "Undeprecated in this fork" after merging this PR. Can we mention this is deprecated in the original package and add a link to that?

https://pkg.go.dev/crypto/x509#CertPool.Subjects

roger2hk · 2025-03-18T12:23:19Z

internal/x509util/ct.go

+		seenCTEKU := false
+		for _, ext := range preIssuer.Extensions {
+			if ext.Id.Equal(oidExtensionKeyUsageCertificateTransparency) {
+				seenCTEKU = true
+				break
+			}
+		}
+		if !seenCTEKU {


Could slices.ContainsFunc be used here?

Ack - will do in a followup PR, this PR is trying no to change the implementation of these functions too much and to copy them over from c-t-go.

use local BuildPrecertTBS instead of ctgo

ec09671

phbnf requested review from roger2hk and AlCutter March 18, 2025 10:55

phbnf assigned roger2hk and AlCutter Mar 18, 2025

phbnf force-pushed the betterfork branch from 83216e1 to 76e7349 Compare March 18, 2025 10:55

phbnf added 11 commits March 18, 2025 10:58

sed ctgo/x509 - crypto/x509

59fb25d

# Conflicts: # internal/scti/storage.go # Conflicts: # internal/scti/ctlog.go # Conflicts: # internal/scti/chain_validation.go # internal/scti/handlers_test.go # internal/scti/signatures_test.go # Conflicts: # ctlog.go

fix requestLog: implements stringer

f88b170

# Conflicts: # internal/scti/requestlog.go

move pem_cert_pool to x509 package

0bd7127

fix a bunch of things

48f2765

# Conflicts: # ctlog.go # Conflicts: # ctlog.go # Conflicts: # internal/scti/chain_validation_test.go

migrate x509util to standard librairies

93ddf29

drop dep on c-t-go/x509 in x509util and delete print funcs

d39f05e

# Conflicts: # internal/x509util/x509util.go # Conflicts: # internal/x509util/x509util.go

remove last IsFatal

dbcaedd

edit isPreIssuer

dffb7a0

# Conflicts: # internal/scti/handlers.go

remove unused PreCertificate method

4a13f68

migrate x509/ct.go away from c-t-go

60433f4

# Conflicts: # internal/x509util/ct.go # internal/x509util/ct_test.go

migrate from x509 to x509fork

ff44c33

phbnf force-pushed the betterfork branch from 76e7349 to e7ecf0d Compare March 18, 2025 11:04

phbnf added 3 commits March 18, 2025 11:12

undeprecate Subject

cb16630

Add comments

c97adc3

migrate hammer

b181276

phbnf force-pushed the betterfork branch from e7ecf0d to b181276 Compare March 18, 2025 11:12

phbnf marked this pull request as ready for review March 18, 2025 11:19

better comments

6c95c19

AlCutter approved these changes Mar 18, 2025

View reviewed changes

roger2hk reviewed Mar 18, 2025

View reviewed changes

roger2hk approved these changes Mar 18, 2025

View reviewed changes

edit comment and log message

f142b99

phbnf merged commit e45de5e into transparency-dev:main Mar 18, 2025
7 checks passed

phbnf mentioned this pull request Apr 4, 2025

split types into rfc6962 and staticctapi #234

Merged

Migrate away from c-t-go/x509 and c-t-go/asn1 #198

Migrate away from c-t-go/x509 and c-t-go/asn1 #198

Uh oh!

Conversation

phbnf commented Mar 18, 2025

Uh oh!

AlCutter left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roger2hk Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

roger2hk Mar 18, 2025 •

edited

Loading